fix(test): fix flaky test related to ec2.updateStatus #5758
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions. |
Hweinstock
changed the title
justinmk3
approved these changes
Oct 10, 2024
Contributor
justinmk3
left a comment
justinmk3
left a comment
There was a problem hiding this comment.
Nice, clear explanation!
tverney
pushed a commit
to tverney/aws-toolkit-vscode
that referenced
this pull request
Oct 21, 2024
## Problem Follow up to: aws#5698 Fix: aws#5750 ### First Problem Identified The current tests in https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/awsService/ec2/explorer/ec2ParentNode.ts do not clear the `PollingSet` in between tests, therefore the timer continues to run. This not only results in inconsistent state between the items in the `PollingSet` and the items stored on the parent node itself, but also allows the `PollingSet` action to trigger unexpectedly in the middle of another test. When this happens, and the states don't match, the `instanceId` from the `PollingSet` could potentially not be found on the parent node, resulting in an undefined node. Then, calling `updateStatus` on this node could explain the error. For this error to happen, the `PollingSet` timer must trigger at a very specific point (in the middle of another test), making it difficult to debug and reproduce, and causing occasional flaky test failures. ### Second Problem Identified The tests in https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/awsService/ec2/explorer/ec2InstanceNode.ts setup an unnatural state. The `testNode` sets its parent to the `testParentNode`, but `testParentNode` is unaware this child exists. This is problematic because when the child reports its status as pending to the parent, the parent will throw an error since it can't find the child. (example: https://d1ihu6zq92vp9p.cloudfront.net/7b1e96c7-f2b7-4a8e-927c-b7aa84437960/report.html) This type of state should not be allowed. ## Solution ### Solution to first problem - clear the set, and the timer in-between each test. ### Solution to second problem - make this confusing state impossible by adding the child to the parent and parent to the child together. - stub the resulting API call its makes. - Disable the polling set timer in the second tests (via sinon stubbing) since it isn't relevant to what's being tested. ### Tangential Work included in PR: - Throw our own error when an `instanceId` is not in map on the parentNode. This will make it easier to catch if something goes wrong. - Clean up test file: `src/test/awsService/ec2/explorer/ec2ParentNode.test.ts`. --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Follow up to: #5698
Fix: #5750
First Problem Identified
The current tests in https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/awsService/ec2/explorer/ec2ParentNode.ts do not clear the
PollingSetin between tests, therefore the timer continues to run. This not only results in inconsistent state between the items in thePollingSetand the items stored on the parent node itself, but also allows thePollingSetaction to trigger unexpectedly in the middle of another test. When this happens, and the states don't match, theinstanceIdfrom thePollingSetcould potentially not be found on the parent node, resulting in an undefined node. Then, callingupdateStatuson this node could explain the error.For this error to happen, the
PollingSettimer must trigger at a very specific point (in the middle of another test), making it difficult to debug and reproduce, and causing occasional flaky test failures.Second Problem Identified
The tests in https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/awsService/ec2/explorer/ec2InstanceNode.ts setup an unnatural state. The
testNodesets its parent to thetestParentNode, buttestParentNodeis unaware this child exists. This is problematic because when the child reports its status as pending to the parent, the parent will throw an error since it can't find the child. (example: https://d1ihu6zq92vp9p.cloudfront.net/7b1e96c7-f2b7-4a8e-927c-b7aa84437960/report.html) This type of state should not be allowed.Solution
Solution to first problem
Solution to second problem
Tangential Work included in PR:
instanceIdis not in map on the parentNode. This will make it easier to catch if something goes wrong.src/test/awsService/ec2/explorer/ec2ParentNode.test.ts.License: I confirm that my contribution is made under the terms of the Apache 2.0 license.